Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: add readableDidRead #36820

Closed
wants to merge 3 commits into from
Closed

stream: add readableDidRead #36820

wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 6, 2021 •
edited
Loading

Adds readableDidRead to streams and applies usage to http.

@ronag ronag added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. http2 Issues or PRs related to the http2 subsystem. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jan 6, 2021
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 6, 2021
@ronag ronag requested a review from lpinca January 6, 2021 20:45
@ronag ronag removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 6, 2021
@ronag ronag marked this pull request as draft January 6, 2021 21:40
@ronag ronag force-pushed the http-manualStart branch 3 times, most recently from 36596e7 to 823908d Compare January 6, 2021 22:32
@ronag

This comment has been minimized.

Sign in to view
@ronag ronag force-pushed the http-manualStart branch 2 times, most recently from eae9165 to f89ab33 Compare January 6, 2021 22:42
@ronag ronag changed the title stream: add manualStart stream: add readableDidRead Jan 6, 2021
@ronag ronag marked this pull request as ready for review January 6, 2021 22:44
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2021
@ronag ronag marked this pull request as draft January 6, 2021 22:59
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35343/

@ronag ronag marked this pull request as ready for review January 6, 2021 23:08
@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed http2 Issues or PRs related to the http2 subsystem. labels Jul 8, 2021
@ronag
Copy link
Member Author

ronag commented Jul 8, 2021

@benjamingr @vweevers

@ronag ronag mentioned this pull request Jul 9, 2021
Closed
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 11, 2021
@ronag ronag requested a review from mcollina July 13, 2021 21:53
mcollina
mcollina approved these changes Jul 14, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/39042/

ronag added a commit that referenced this pull request Jul 14, 2021
@ronag
stream: add readableDidRead
8306051 
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: #36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ronag
Copy link
Member Author

ronag commented Jul 14, 2021

Landed in 8306051

@ronag ronag closed this Jul 14, 2021
targos pushed a commit that referenced this pull request Jul 17, 2021
@ronag @targos
stream: add readableDidRead
8fe7d23 
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: #36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
Merged
@targos
Copy link
Member

targos commented Jul 27, 2021

Is it intended to be public API? In that case it should be documented and this PR is semver-minor.

@targos
Copy link
Member

targos commented Jul 29, 2021

@ronag ^

@ronag ronag added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 29, 2021
ronag added a commit to nxtedition/node that referenced this pull request Jul 29, 2021
@ronag
stream: add readableDidRead
c69dab9 
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ronag
Copy link
Member Author

ronag commented Jul 30, 2021 •
edited
Loading

Added don't land tags for now due to ongoing discussions.

ronag added a commit to nxtedition/node that referenced this pull request Aug 1, 2021
@ronag
stream: add readableDidRead
5ef4cda 
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos
Copy link
Member

targos commented Aug 22, 2021

Where are the discussions happening?

@targos
Copy link
Member

targos commented Aug 22, 2021

Oh I see, it was reverted and implemented differently in #39589

ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2021
@ronag
stream: add readableDidRead
f0201c1 
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2021
@ronag
stream: add readableDidRead
9bd7492 
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@apapirovski apapirovski Awaiting requested review from apapirovski

@lpinca lpinca Awaiting requested review from lpinca

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronag @nodejs-github-bot @mscdex @jasnell @targos @mcollina @benjamingr